-
Notifications
You must be signed in to change notification settings - Fork 193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix negotiate authentication between CGIs and scheduler #19
Fix negotiate authentication between CGIs and scheduler #19
Conversation
SetAuthorizationString with NULL argument sets an empty string. Related: apple#5596 Signed-off-by: Samuel Cabrero <scabrero@suse.de>
Related: apple#5596 Signed-off-by: Samuel Cabrero <scabrero@suse.de>
If connecting to localhost then proceed to ask the client for the authorization using cupsGetPassword2. The get password callback will return 401 to the client with WWW-Authenticate: Negotiate. Fixes: apple#5596 Signed-off-by: Samuel Cabrero <scabrero@suse.de>
PeerCred is also possible if address family is AF_LOCAL. This will allow the CGI programs to generate the authorization from the local certificates based on PID also when Negotiate is used for local connections: Client CGI Browser <- Remote conn -> admin.cgi <--- Localhost conn ---> Scheduler | | | + --- HTTP/POST /admin/ --> | | | + --- CUPS-Get-Devices ------------> | | | | | | <-- 401 Unauthorized --------------+ | | WWW-Authenticate: | | | Negotiate, (PeerCred,) Local | | | | | <-- 401 Unauthorized -----+ | | WWW-Authenticate: | | | Negotiate | | | | | | --- HTTP/POST /admin/ --> | | | Authorization: + --- IPP CUPS-GetDevices ---------> | | Negotiate | Authorization: Local <cert> | | | | Fixes: apple#5596 Signed-off-by: Samuel Cabrero <scabrero@suse.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to do more in-depth testing for this because the interactions between Kerberos and local/peer credentials/authref are a constant source of pain.
@@ -2109,18 +2109,13 @@ cupsdSendHeader( | |||
} | |||
else if (auth_type == CUPSD_AUTH_NEGOTIATE) | |||
{ | |||
#if defined(SO_PEERCRED) && defined(AF_LOCAL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break macOS clients. We never send Kerberos credentials over a domain socket for a native (IPP) client since we use the login UID to do authorization. Need to think about how this can be managed... :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't aware how these changes can affect macOS.
The 'PeerCred' mech will still be in WWW-Authenticate string together with 'Negotiate' and 'Local', added just below:
https://github.com/scabrero/cups/blob/web-interface-negotiate-fix/scheduler/client.c#L2135
Then the cups_local_auth
will work also for macOS because of this check:
if (http->hostaddr->addr.sa_family == AF_LOCAL &&
!getenv("GATEWAY_INTERFACE") && /* Not via CGI programs... */
cups_auth_find(www_auth, "PeerCred"))
{
/* 1 if local connection */ | ||
cups_is_local_connection(http_t *http) /* I - HTTP connection to server */ | ||
{ | ||
if (!httpAddrLocalhost(http->hostaddr) && _cups_strcasecmp(http->hostname, "localhost") != 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can probably just be:
return (httpAddLocalhost(http->hostaddr) || !_cups_strcasecmp(http->hostname, "localhost"));
Hi Michael, could I ask the status of this? Is it still under evaluation? Do you require anything from my side to move forward? |
@scabrero Yes, otherwise I would have closed it with a reason. Like I said in my original review, I need time to ensure this doesn't cause regressions. This includes setting up a test environment with a Kerberos/AD server, which I no longer have easily available in a test lab, and setting up a macOS VM where I can muck around with the root partition. This isn't a full-time, paid gig for me anymore, I'm volunteering my time and computer resources to help keep CUPS moving forward. So please be patient - some PRs and issues take longer to review and verify. |
OK, I'm going to merge this change. If it causes problems on macOS and Apple actually uses our fork, they can fix it. Also, I should note that we are deprecating Kerberos support in CUPS 2.4.0, with OAuth coming to pick up the SSO slack. |
Fixes the infinite loop when "Negotiate" authentication is used and allows "Local" certificate based authentication when connecting to localhost:
Fixes apple/cups issue #5596